fix: Issues with Modal and Image Cropper position #5367

sachinchauhan2889 · 2020-10-23T17:23:58Z

Fixes #4837, #4917, #4716, #4505, #5308

Short description of what this resolves:

this PR can solve issue #5308 #4917 #4837 #4716 #4505 .

changes

fix Organizer Session Notifications: Message Box if Off-Screen #4837 now session notify modal is perfectly placed.
fix Interface error when organizer uploads speaker's image #4917 Cropper modal(for speaker image) is positioned right.
fix Cropper modal in event-create wizard is not correctly positioned #4716 image cropper(for event creation) is positioned right.
fix Tax information pop up in Wizard step 1 showing up towards bottom of screen #4505 Tax information modal is positioned well. It have close icon on right top. It can open and close multiple times.
fix Speaker Notification: HTML code in message and pop up not entirely visible #5308 session notify modal is perfectly visible.

I have tested all modals and croppers mentioned above from all scroll positions and get modal at center.

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

screenshots

vercel · 2020-10-23T17:26:58Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/jwdz7zmdy
✅ Preview: https://open-event-frontend-git-align-sessions-notify-modal-4837.eventyay.now.sh

iamareebjamal · 2020-10-23T18:43:14Z

app/components/modals/modal-base.js

+      if (this.noCropper) {
+        $element.modal({
+          centered: false
+        }).modal('show');


This will ignore all the settings done in the modal below. As evident by the dark dimmer in the modal. Make this change in the settings below

iamareebjamal · 2020-10-23T18:43:45Z

app/components/modals/session-notify-modal.js

@@ -16,6 +16,7 @@ export default class SessionNotifyModal extends ModalBase {

  constructor() {
    super(...arguments);
+    this.noCropper = true;


Pass this as a prop rather than setting here

sachinchauhan2889 · 2020-10-23T18:48:06Z

@iamareebjamal thank you for your reviews. i am working on changes suggested by you.

iamareebjamal · 2020-10-24T08:28:53Z

Like my review explained why changing it this way is not good enough, @mariobehling has said

Thank you!

Could you keep the overlay background semitransparent white, please for consistency?
Please make the X button black (on white background).
When a user closes the pop up using the X button and then clicks add tax information again nothing happens. Expected: It should open the pop up again.

sachinchauhan2889 · 2020-10-24T09:33:13Z

This will ignore all the settings done in the modal below. As evident by the dark dimmer in the modal. Make this change in the settings below

@iamareebjamal i try to make changes to settings in willInitSemantic(settings){...} function.
But centered property of modal not assigned with false. I think Settings of semantic ui modal not have any property named centered. https://semantic-ui.com/modules/modal.html#/settings

sachinchauhan2889 · 2020-10-24T09:34:47Z

@iamareebjamal can i assign all properties here.

$element.modal({
centered: false
}).

iamareebjamal · 2020-10-24T09:39:58Z

It should have the same interface of settings, but if it is not working, you can try that

sachinchauhan2889 · 2020-10-24T10:15:40Z

@iamareebjamal please have a look at this.
Now this PR can solve issue #5308 #4917 #4837 #4716 #4505 .

changes

fix Organizer Session Notifications: Message Box if Off-Screen #4837 now session notify modal is perfectly placed.
fix Interface error when organizer uploads speaker's image #4917 Cropper modal(for speaker image) is positioned right.
fix Cropper modal in event-create wizard is not correctly positioned #4716 image cropper(for event creation) is positioned right.
fix Tax information pop up in Wizard step 1 showing up towards bottom of screen #4505 Tax information modal is positioned well. It have close icon on right top. It can open and close multiple times.
fix Speaker Notification: HTML code in message and pop up not entirely visible #5308 session notify modal is perfectly visible.

I have tested all modals and croppers mentioned above from all scroll positions and get modal at center.

screenshots

fix-all-modals-crpper chore(deps-dev): bump @sentry/tracing from 5.27.0 to 5.27.1 (fossasia#5372) Bumps [@sentry/tracing](https://github.com/getsentry/sentry-javascript) from 5.27.0 to 5.27.1. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@5.27.0...5.27.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> chore(deps-dev): bump mini-css-extract-plugin from 1.1.2 to 1.2.0 (fossasia#5371) Bumps [mini-css-extract-plugin](https://github.com/webpack-contrib/mini-css-extract-plugin) from 1.1.2 to 1.2.0. - [Release notes](https://github.com/webpack-contrib/mini-css-extract-plugin/releases) - [Changelog](https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/CHANGELOG.md) - [Commits](webpack-contrib/mini-css-extract-plugin@v1.1.2...v1.2.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

iamareebjamal · 2020-10-24T10:27:54Z

app/components/modals/modal-base.js

  classNameBindings : ['isFullScreen:fullscreen', 'isSmall:small', 'isLarge:large'],

  openObserver: observer('isOpen', function() {
    const $element = $(this.element);
    if (this.isOpen) {
-      $element.modal('show');
+      $element.modal({


Copied code. Move to a single function

iamareebjamal · 2020-10-24T10:28:20Z

app/components/modals/modal-base.js

@@ -91,7 +129,44 @@ export default UiModal.extend({

  didInitSemantic() {
    if (this.isOpen) {
-      $(this.element).modal('show');
+      $(this.element).modal({


Duplicate code. Remove any duplicate code

iamareebjamal · 2020-10-24T10:28:50Z

app/templates/components/modals/tax-info-modal.hbs

@@ -1,3 +1,4 @@
+<i class="black close icon"></i>


There is already a closable icon in the modal I believe?

@iamareebjamal not present earlier. please see #4505

… into align-sessions-notify-modal-fossasia#4837

sachinchauhan2889 · 2020-10-24T11:59:42Z

@iamareebjamal please have a look at this PR. i have removed Duplicacy of code.

iamareebjamal · 2020-10-24T12:07:33Z

app/components/modals/modal-base.js

  classNameBindings : ['isFullScreen:fullscreen', 'isSmall:small', 'isLarge:large'],

  openObserver: observer('isOpen', function() {
    const $element = $(this.element);
    if (this.isOpen) {
-      $element.modal('show');
+      $element.modal({...this.defaultOptions}).modal('show');


Suggested change

$element.modal({...this.defaultOptions}).modal('show');

$element.modal(this.defaultOptions).modal('show');

iamareebjamal · 2020-10-24T12:07:59Z

app/components/modals/modal-base.js

    assign(settings, options);
  },

  didInitSemantic() {
    if (this.isOpen) {
-      $(this.element).modal('show');
+      $(this.element).modal({...this.defaultOptions}).modal('show');


Suggested change

$(this.element).modal({...this.defaultOptions}).modal('show');

$(this.element).modal(this.defaultOptions).modal('show');

iamareebjamal · 2020-10-24T12:12:24Z

Since now you're only using the default options of the modal, what's the need for passing them in the modal(this.defaultOptions) call? Try removing them and try again, it should work without passing the settings

sachinchauhan2889 · 2020-10-24T12:24:39Z

Since now you're only using the default options of the modal, what's the need for passing them in the modal(this.defaultOptions) call? Try removing them and try again, it should work without passing the settings

@iamareebjamal sir i have tried this but modal is not appearing at center. All properties are present in settiings of modal but centered: false is not present in settings. For this centered: false property i have used defaultOptions object and pass it here.

I think this is because the settings of semantic ui modal not contain centered as property. This is why i have make defaultOptions object in init() function and use this object in $(this.element).modal(defaultOptions).modal('show').

$(this.element).modal('show') is not working.

i have done all changes suggested by you. have a look at this.

iamareebjamal · 2020-10-24T13:15:15Z

Checking

iamareebjamal · 2020-10-24T14:47:03Z

Good job. But there are still some issues. I am mentioning them one by one.

#4837: If you decrease the height of the screen a little bit, the modal shifts to right

Test on heights < 825px

Same issue in #4917, #4716, #4505

iamareebjamal · 2020-10-24T14:56:10Z

I think your styles are being overriden in a media query

iamareebjamal · 2020-10-24T15:03:35Z

When the screen is smaller in height than 825px, semantic adds left: 50% which shifts the dialog further to the right. You need to add left:auto !important in your CSS style to make sure it stays in center at any height

codecov · 2020-10-24T15:23:41Z

Codecov Report

Merging #5367 into development will decrease coverage by 0.05%.
The diff coverage is 50.00%.

@@               Coverage Diff               @@
##           development    #5367      +/-   ##
===============================================
- Coverage        22.78%   22.72%   -0.06%     
===============================================
  Files              491      491              
  Lines             5245     5245              
  Branches            37       37              
===============================================
- Hits              1195     1192       -3     
- Misses            4045     4048       +3     
  Partials             5        5

Impacted Files	Coverage Δ
app/components/modals/tax-info-modal.js	`22.22% <ø> (ø)`
app/components/modals/modal-base.js	`25.00% <50.00%> (ø)`
app/models/event.js	`31.25% <0.00%> (-18.75%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03422cf...6c5946c. Read the comment docs.

iamareebjamal · 2020-10-24T15:25:08Z

Thanks a lot for solving a complex and long standing issue with little code changes elegantly

sachinchauhan2889 · 2020-10-24T17:24:14Z

Thanks a lot for solving a complex and long standing issue with little code changes elegantly

@iamareebjamal my pleasure sir

auto-label bot added the fix label Oct 23, 2020

vercel bot deployed to Preview October 23, 2020 17:24 View deployment

iamareebjamal reviewed Oct 23, 2020

View reviewed changes

This was referenced Oct 24, 2020

fix Tax information pop up in Wizard step 1 showing up towards bottom of screen #5368

Closed

fix: Speaker Notification: HTML code in message and pop up not entirely visible #5366

Closed

vercel bot deployed to Preview October 24, 2020 10:01 View deployment

sachinchauhan2889 changed the title ~~fix: Organizer Session Notifications: Message Box if Off-Screen~~ fix: Issues with Modal and image Cropper position Oct 24, 2020

vercel bot deployed to Preview October 24, 2020 10:19 View deployment

sachinchauhan2889 force-pushed the align-sessions-notify-modal-#4837 branch from 48455af to a9805f1 Compare October 24, 2020 10:23

vercel bot deployed to Preview October 24, 2020 10:24 View deployment

vercel bot deployed to Preview October 24, 2020 10:26 View deployment

iamareebjamal reviewed Oct 24, 2020

View reviewed changes

sachinchauhan2889 added 2 commits October 24, 2020 17:18

remove-duplicacy

7afde0a

Merge branch 'development' of github.com:fossasia/open-event-frontend…

0833e7d

… into align-sessions-notify-modal-fossasia#4837

sachinchauhan2889 force-pushed the align-sessions-notify-modal-#4837 branch from 408c731 to 0833e7d Compare October 24, 2020 11:53

vercel bot deployed to Preview October 24, 2020 11:53 View deployment

final

1a68049

vercel bot deployed to Preview October 24, 2020 11:56 View deployment

iamareebjamal reviewed Oct 24, 2020

View reviewed changes

remove spread

b17fc08

vercel bot deployed to Preview October 24, 2020 12:13 View deployment

sachinchauhan2889 requested a review from iamareebjamal October 24, 2020 12:31

Update overrides.scss

6c5946c

vercel bot deployed to Preview October 24, 2020 15:10 View deployment

iamareebjamal approved these changes Oct 24, 2020

View reviewed changes

iamareebjamal changed the title ~~fix: Issues with Modal and image Cropper position~~ fix: Issues with Modal and Image Cropper position Oct 24, 2020

iamareebjamal merged commit 7971c4a into fossasia:development Oct 24, 2020

sachinchauhan2889 deleted the align-sessions-notify-modal-#4837 branch October 26, 2020 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Issues with Modal and Image Cropper position #5367

fix: Issues with Modal and Image Cropper position #5367

sachinchauhan2889 commented Oct 23, 2020 •

edited by iamareebjamal

Loading

vercel bot commented Oct 23, 2020 •

edited

Loading

iamareebjamal Oct 23, 2020

iamareebjamal Oct 23, 2020

sachinchauhan2889 commented Oct 23, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal Oct 24, 2020

iamareebjamal Oct 24, 2020

iamareebjamal Oct 24, 2020

sachinchauhan2889 Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal Oct 24, 2020

iamareebjamal Oct 24, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020 •

edited

Loading

iamareebjamal commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

codecov bot commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

	$element.modal({...this.defaultOptions}).modal('show');
	$element.modal(this.defaultOptions).modal('show');

	$(this.element).modal({...this.defaultOptions}).modal('show');
	$(this.element).modal(this.defaultOptions).modal('show');

fix: Issues with Modal and Image Cropper position #5367

fix: Issues with Modal and Image Cropper position #5367

Conversation

sachinchauhan2889 commented Oct 23, 2020 • edited by iamareebjamal Loading

Short description of what this resolves:

Checklist

vercel bot commented Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachinchauhan2889 commented Oct 23, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachinchauhan2889 commented Oct 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020 • edited Loading

iamareebjamal commented Oct 24, 2020

iamareebjamal commented Oct 24, 2020

codecov bot commented Oct 24, 2020

Codecov Report

iamareebjamal commented Oct 24, 2020

sachinchauhan2889 commented Oct 24, 2020

sachinchauhan2889 commented Oct 23, 2020 •

edited by iamareebjamal

Loading

vercel bot commented Oct 23, 2020 •

edited

Loading

iamareebjamal commented Oct 24, 2020 •

edited

Loading